Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixes #7055: Fix reporting issue (multiple reports for one component) on branch 2.11 #902

Closed
wants to merge 1 commit into from
Closed

Fixes #7055: Fix reporting issue (multiple reports for one component) on branch 2.11 #902

wants to merge 1 commit into from

Conversation

VinceMacBuche
Copy link
Member

@VinceMacBuche
Copy link
Member Author

PR updated

@VinceMacBuche VinceMacBuche force-pushed the bug_7055/fix_reporting_issue_multiple_reports_for_one_component_on_branch_2_11 branch from 14bee58 to d811af3 Compare July 30, 2015 14:42
ncharles
ncharles reviewed Jul 30, 2015
View reviewed changes
rudder-core/src/main/scala/com/normation/rudder/domain/reports/bean/ExecutionBatch.scala
val componentValues = components.flatMap(_.componentValues.filter(v => values.contains(v.componentValue))).groupBy(_.componentValue)
for {
// for each value, process a reporting date
(value,values) <- componentValues
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about those that don't match ?
plus, you are using twice values in 3 lines, so when rereading, it's hard to know which one we use

@VinceMacBuche
Copy link
Member Author

PR updated

@VinceMacBuche VinceMacBuche force-pushed the bug_7055/fix_reporting_issue_multiple_reports_for_one_component_on_branch_2_11 branch from d811af3 to 24ee5fa Compare July 30, 2015 15:16
@VinceMacBuche
Copy link
Member Author

PR updated

@VinceMacBuche VinceMacBuche force-pushed the bug_7055/fix_reporting_issue_multiple_reports_for_one_component_on_branch_2_11 branch from 24ee5fa to b12d798 Compare July 30, 2015 15:28
@VinceMacBuche
Copy link
Member Author

PR updated

@VinceMacBuche VinceMacBuche force-pushed the bug_7055/fix_reporting_issue_multiple_reports_for_one_component_on_branch_2_11 branch from 5a02c05 to 72b1d31 Compare July 30, 2015 17:44
@fanf
Copy link
Member

fanf commented Aug 5, 2015

OK, so there is a lots of things strange or that I don't understand on the pr. For one, the tests are not passing. Second: it seems that we don't have values anymore, at all, in ComponentStatusReport, but only the unexpanded value. I'm pretty sure we don't want to throw it away (and only rely on the content of reports to hope for it).
I spent several hours trying to understand what we really want, and I'm not sure of the answer now. I think that one of the reason is that our data model is not really clear, and we really miss a "report key" - what kind of unexpandedValue is used for, here, but not quite because we are still unable to correctly merge back reports on lots of edge cases.
Redisigning the data model to assert that a we have "unexpandedvalue that can have several values that can have several messages" is a rather big change, not sure we want it to go in 3.0.

Or we assume the fact that we are only managing unexpanded values and that we loose the value along the way, and we address the problem in 3.2.

Ideas ?

@fanf
Copy link
Member

fanf commented Aug 6, 2015

Ignore the preceding comment, it was for branch 3.0 / pr #904

@fanf fanf mentioned this pull request Aug 6, 2015
Merged
@fanf
Copy link
Member

fanf commented Aug 6, 2015

Closing this one because need a rebase, replaced by #911

@fanf fanf closed this Aug 6, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@VinceMacBuche @fanf @ncharles